Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] split and join #96156

Merged
merged 5 commits into from
May 4, 2020
Merged

[WIP] split and join #96156

merged 5 commits into from
May 4, 2020

Conversation

kieferrm
Copy link
Member

@kieferrm kieferrm commented Apr 25, 2020

Implements a first draft of split and join of notebook cells.

It only works for (neigboring) code cells. Although there is nothing in the code that should prevent joining a code and a markdown cell setText on a markdown cell does not have any affect. For the same reason splitting of markdown cells does not work. Additionally, that a markdown cell is rendered when it loses focus is not what I'd expect when splitting it.

Modifying the content of cells is bad. For example, when splitting a cell, we should just be able to delete some content rather than resetting it. Also, we get content as a string of joined lines only to split it again after joining it with other content. Also, I'm not sure that the funtionality even lives at the right layer.

@kieferrm kieferrm changed the title split and join [WIP] split and join Apr 25, 2020
@kieferrm kieferrm requested a review from rebornix April 25, 2020 23:27
@rebornix
Copy link
Member

really good work and the issues raised are valid. Ideally the logic can implemented all in coreActions but currently we can't due to following limitations in the core

  • Modifying cell content: right now we will create a text buffer when doing search for every cell and attach TextModel to the cell once it's visible. We should probably expose an ITextBuffer like interface to support rich text editing other than simple set
  • Layering: putting the split/join actions in either the NotebookEditor or NotebookViewModel are both fine. Using the latter is better as we then only touch the viewModel/textModel code and the UI will be updated properly automatically. One missing feature is merging undo points. Deleting a cell and then inserting one are two separate undo points now.
    • We need to add pushUndoStop API like what Monaco Editor does, to allow contribs control how operations should be merged
    • Join Cells is a bit difficult as cell operation and cell content change mixed together.

@rebornix
Copy link
Member

If we are going to implement this feature in an extension, the missing APIs are:

  • Shortcut to get TextEditor for a notebook cell if it exist. Currently extensions have to go through all editors and check if their resource URI matches. (but it's not necessarily a bad idea)
  • Reveal/Focus cell API. After a cell is split or joined, extensions may need to set selections correctly.
    • For joining two cells, the text editor for the first cell should not change and there should only be a modelContentChange event emitted.

@rebornix rebornix mentioned this pull request May 1, 2020
17 tasks
@kieferrm
Copy link
Member Author

kieferrm commented May 2, 2020

@rebornix Based on our discussion, I reworked the PR so that we now work directly with lines, i.e. string[]. I added a todo in the code where we'd need to make sure we don't have to join the line contents again. This is a bit more complicated but avoids copying content unnecessarily. It also allows to work with positions directly.

@kieferrm
Copy link
Member Author

kieferrm commented May 2, 2020

Pls merge if you feel that's good enough for now before you rework the cell text buffers.

@rebornix
Copy link
Member

rebornix commented May 4, 2020

@kieferrm thanks, it looks great. I'll fix the conflicts and merge.

@rebornix rebornix merged commit ec616b3 into master May 4, 2020
@rebornix rebornix deleted the kieferrm/split-cell branch May 4, 2020 18:05
@github-actions github-actions bot locked and limited conversation to collaborators Jun 18, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants